Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake fixes to support Windows builds. #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

CMake fixes to support Windows builds. #1

wants to merge 6 commits into from

Conversation

miishuistr
Copy link

I found the CMake suppport for Windows lacking, and I decided to try to improve it. Please let me know what additional modifications I need to make for a reliable windows+cmake build

* [Execution, SymbolicEnvironment] Correct _cdecl adn _stdcall definitions
* [VirtualMemory] Renamed structure MemoryRegionInfo so it doesn't collide with enum value
* [revtracer-wrapper] Added compile time definition

Everything compiles now on windows!
* [BinLoader] added Extern.Mapper.cpp and Mem.Mapper.cpp to cmake.
* [Execution, loader, revtracer-wrapper] Specified OS-dependant libs.
* [Execution] Removed redundant ifdef in Main.cpp
CMakeLists.txt Outdated
@@ -1,8 +1,14 @@
cmake_minimum_required(VERSION 2.8)
set(CMAKE_SYSTEM_NAME Linux)
cmake_minimum_required(VERSION 3.7)
Copy link
Contributor

@alexandrasandulescu alexandrasandulescu Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubuntu supports cmake 3.5 in the official repositories. What is the reason behind 3.7? We need to upgrade all the machines with RIVER for this change otherwise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the requirement for cmake 3.7. I strongly suggest setting a DESTDIR when running cmake since the default DESTDIR for Windows is %programfiles%.

@@ -1,16 +1,25 @@
## Execution CMakeLists.txt

set(LIBRARY_NAME execution)
set(FLAGS_CROSS "-D__cdecl=\"\" -D__stdcall=\"\"")
if(WIN32)
Copy link
Contributor

@alexandrasandulescu alexandrasandulescu Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this if statement sets the previous version on LINUX and your version on WIN

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it weird that calling conventions get smashed on linux. Are you sure this is your intention?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work with __attribute__((stdcall)) and __attribute__((cdecl)). But I see the definition is for _cdecl and _stdcall. The previous code uses __stdcall and __cdecl. Check for example revtracer/revtracer.h:70

@alexandrasandulescu
Copy link
Contributor

These changes can help us extend the tracer functionality on Windows. Very good initiative. I have few extra comments:

  1. please pay attention to indentation. We used this setup for .vimrc.
    set expandtab set tabstop=4 set softtabstop=4 set shiftwidth=4

  2. The Linux build fails because Extern.Mapper.cpp and Mem.Mapper.cpp are not cross-compatible. It also fails because of stdcall cdecl specifiers macros I mentioned in the comments.

  3. I would like to start the build on a Windows machine. What are the required packages and what is the cmake command that you use on Windows. Please document this in Readme.

Quickly documented Windows build steps
@miishuistr
Copy link
Author

  1. Will follow up with a commit disabling Extern.Mapper.cpp and Mem.Mapper.cpp from the Linux build.
  2. Steps are enumerated in BUILD_INSTRUCTIONS_WINDOWS.txt. Currently I'm using CMake 3.12.3 (although 2.8 will suffice) and Visual Studio Community 2017 (older versions might also work).

@alexandrasandulescu
Copy link
Contributor

Windows build worked. Please disable benchmarking-payload from windows build.

@alexandrasandulescu
Copy link
Contributor

The next actions points are:

  • remove Win sources from BinLoader makefile
  • fix attributes for linux
  • disable benchmarking-payload for win

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants